Add transport worker factory and abstract base class for background worker #4580
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## srothh/transport-class-hierarchy #4580 +/- ##
====================================================================
+ Coverage 83.70% 84.98% +1.28%
====================================================================
Files 158 158
Lines 15395 15414 +19
Branches 2432 2432
====================================================================
+ Hits 12886 13100 +214
+ Misses 1755 1561 -194
+ Partials 754 753 -1
|
The uncovered lines of code here are just the pass calls from the abstract class definition. |
srothh
force-pushed
the
srothh/transport-class-hierarchy
branch
from
53b92f2 to
3607d44
Compare
srothh
force-pushed
the
srothh/worker-class-hierarchy
branch
2 times, most recently
from
2896602 to
268ea1a
Compare
antonpirker
approved these changes
Jul 24, 2025
| @property | ||
| @abstractmethod | ||
| def is_alive(self) -> bool: | ||
| pass |
There was a problem hiding this comment.
Please add docstrings to all methods in the new abstract worker class.
srothh
force-pushed
the
srothh/transport-class-hierarchy
branch
from
062ab5b to
4e56e5c
Compare
Add an abstract bass class for implementation of the background worker. This was done to provide a shared interface for the current implementation of a threaded worker in the sync context as well as the upcoming async task-based worker implementation. GH-4578
Add a new factory method instead of direct instatiation of the threaded background worker. This allows for easy extension to other types of workers, such as the upcoming task-based async worker. GH-4578
Add a new flush_async method to worker ABC. This is necessary because the async transport cannot use a synchronous blocking flush. GH-4578
Move the flush_async down to the concrete subclass to not break existing testing. This makes sense, as this will only really be needed by the async worker anyway and therefore is not shared logic. GH-4578
Coroutines have a return value, however the current function signature for the worker methods does not accomodate for this. Therefore, this signature was changed. GH-4578
srothh
force-pushed
the
srothh/worker-class-hierarchy
branch
from
1fbf85f to
ef780f3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a new abstract base class for the background worker implementations in the transport. This allows for implementation of the upcoming async task-based worker in addition to the current thread based worker used in the sync context. Additionally, add a factory method in the HttpTransportCore shared class, to allow the worker methods to live higher up in the class hierarchy regardless of specific implementation.
GH-4578